Skip to content

Conversation

@igorniebylski
Copy link
Contributor

@igorniebylski igorniebylski requested review from Copilot and solatis May 26, 2025 08:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.


@pytest.fixture
def gen_index(start_date, row_count):
def gen_index(start_date, row_count, frequency):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is gen_index invoked? Would it not require a default value for this parameter? Did you check the tests pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a leftover from test_dask.py
above this line i defined frequency fixture
from it you can change value passed to gen_index if needed
default behawior is the same as before, all tests pass
its not useful right, can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for test_dask.py i needed to have data split between multiple shards
defining additional fixture provides way to override behavior for selected tests only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I get it, it's a fixture and we have a separate frequency fixture as well which is automatically injected.

Please keep conftest.py as in-sync as possible with the one in our dask repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i will update conftest in qdb-dask-integration to match

return py::cast(qdb::numpy_query(_handle, query));
}

py::object split_query_range(std::chrono::system_clock::time_point start, std::chrono::system_clock::time_point end, std::chrono::milliseconds delta)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is not too useful right now, I assume you do the regex part in the dask connector now, which is probably not ideal.

Let's keep it as-is though.

@igorniebylski igorniebylski merged commit 1b84ce6 into master May 27, 2025
2 checks passed
@igorniebylski igorniebylski deleted the sc-16714/dask-integration-into-python-api branch May 27, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants